Update npm lockfile 3 ancestor traversal to properly detect dependency roots#1677
Open
FernandoRojo wants to merge 1 commit intomainfrom
Open
Update npm lockfile 3 ancestor traversal to properly detect dependency roots#1677FernandoRojo wants to merge 1 commit intomainfrom
FernandoRojo wants to merge 1 commit intomainfrom
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug in the npm lockfile v3 detector where dependency resolution failed to properly traverse ancestor paths when looking for hoisted dependencies. The previous implementation attempted to reconstruct ancestor paths from component IDs in the dependency graph, which could skip valid ancestor locations. The new implementation correctly walks the lockfile path hierarchy using string manipulation to find dependencies in ancestor node_modules directories before falling back to top-level resolution.
Changes:
- Refactored
EnqueueNestedDependenciesmethod to use path-based ancestor traversal instead of dependency-graph-based traversal - Removed unused
ISingleFileComponentRecorderparameter fromEnqueueNestedDependencies - Added comprehensive test case to verify ancestor path resolution for nested dependencies
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/Microsoft.ComponentDetection.Detectors/npm/NpmLockfile3Detector.cs |
Refactored dependency resolution to walk lockfile paths upward using LastIndexOf("/node_modules/") instead of reconstructing paths from component IDs; removed unused parameter |
test/Microsoft.ComponentDetection.Detectors.Tests/NpmLockfile3DetectorTests.cs |
Added test case TestNpmDetector_ResolvesDependencyFromAncestorPathV3Async to verify correct resolution of dependencies from ancestor paths (A -> B -> C where both B and C are nested under A) |
| this.Logger.LogDebug("Found nested dependency {Dependency} in {AncestorNodeModulesPath}", dep.Key, ancestorNodeModulesPath); | ||
| queue.Enqueue((nestedPkg.Path, nestedPkg.Package, parent)); | ||
| found = true; | ||
| continue; |
There was a problem hiding this comment.
The continue statement here is redundant because found is set to true and the while loop condition checks !found, which will cause the loop to exit naturally. The continue will skip the remaining iterations, but since the loop is already exiting, this doesn't add value. Consider removing this line for clarity.
Suggested change
| continue; |
grvillic
reviewed
Feb 27, 2026
|
|
||
| public override string Id => "NpmLockfile3"; | ||
|
|
||
| public override int Version => 3; |
Collaborator
There was a problem hiding this comment.
Remember to bump detector version
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1677 +/- ##
=====================================
Coverage 90.8% 90.8%
=====================================
Files 451 451
Lines 40144 40204 +60
Branches 2443 2443
=====================================
+ Hits 36456 36517 +61
Misses 3188 3188
+ Partials 500 499 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
RushabhBhansali
previously approved these changes
Mar 3, 2026
Contributor
|
Have we investigated why are verification tests failing for Dotnet Components. Dotnet detector is not being touched in this PR. |
Contributor
RushabhBhansali
left a comment
RushabhBhansali
left a comment
There was a problem hiding this comment.
Have we investigated why are verification tests failing for Dotnet Components. Dotnet detector is not being touched in this PR.
Expected foundComponent to be True because The component for DotNet-10.0.102 netstandard1.4 unknown - DotNet was not present in the new manifest file. Verify this is expected behavior before proceeding, but found False.
Expected foundComponent to be True because The component for DotNet-10.0.102 net40 unknown - DotNet was not present in the new manifest file. Verify this is expected behavior before proceeding, but found False.
Expected foundComponent to be True because The component for DotNet-10.0.102 net45 unknown - DotNet was not present in the new manifest file. Verify this is expected behavior before proceeding, but found False.
Expected foundComponent to be True because The component for PipReport-certifi 2026.1.4 - pip was not present in the new manifest file. Verify this is expected behavior before proceeding, but found False.
Expected foundComponent to be True because The component for DotNet-10.0.103 netstandard1.4 unknown - DotNet was not present in the old manifest file. Verify this is expected behavior before proceeding, but found False.
Expected foundComponent to be True because The component for DotNet-10.0.103 net40 unknown - DotNet was not present in the old manifest file. Verify this is expected behavior before proceeding, but found False.
Expected foundComponent to be True because The component for DotNet-10.0.103 net45 unknown - DotNet was not present in the old manifest file. Verify this is expected behavior before proceeding, but found False.
Expected foundComponent to be True because The component for NpmLockfile3-yallist 4.0.0 - Npm was not present in the old manifest file. Verify this is expected behavior before proceeding, but found False.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
NpmLockfile3Detector had a path-resolution bug when resolving nested dependencies in lockfile v3.
The previous logic tried to reconstruct ancestor lookup paths from dependency-graph component IDs, inproperly traversing combinations of ancesters, instead of walking real lockfile package paths. That can produce invalid candidates and miss valid packages that are hoisted/placed under an ancestor path.
Root cause: ancestor traversal was based on component identity and an erroneous ancestor path determination., not the current lockfile path hierarchy.
Effect: a dependency can be reported as “not found” (or resolved to an incorrect fallback) even though it exists in packages.
Given:
node_modules/A
node_modules/A/node_modules/B
node_modules/A/node_modules/C ← C exists under ancestor A, not under B or root
If B declares dependency C, correct npm-style resolution is:
check
node_modules/A/node_modules/B/node_modules/C (missing)
then node_modules/A/node_modules/C (present, should resolve here)
then root node_modules/C (only if still missing)
The buggy approach could skip step 2 due to path reconstruction from component IDs, causing a false miss.
Fix summary
Resolution now walks from [currentPath] upward through real .../node_modules/... ancestors, checking each candidate path before top-level fallback.
This aligns lookup behavior with npm module resolution semantics and prevents false “missing dependency” outcomes in ancestor-hoisted layouts.